test: cover the shipped surface, and make the coverage figure mean something - #30
Conversation
The modules under test/support are fixtures, and a fixture is covered by being run at all - so counting them moved the total without their ever being the thing measured. Two moved it down for a reason that is not about the suite: Castle.PeerProviderStub and most of Castle.IoSink run in the peer VM, which is a separate node with no cover on it. The threshold is explicit at 85 because the default of 90 is unreachable by construction here. Everything below the "## In the peer" comment in Castle.Peer runs in that same VM, so around 7% of the shipped lines execute on every peer test and are counted as missed, putting the ceiling near 93%. Left at the default, mix test --cover reports a failure no test can fix. It is deliberately left out of mix precommit: a merge-blocking percentage cannot tell a test that discriminates from one written to raise the number. Co-Authored-By: Claude <noreply@anthropic.com>
The refusal for a marker path already occupied describes what it found,
and describe_type/1 fell through to "a #{type}" for anything that is not
a directory or a symlink. File.lstat/1 answers :other for a named pipe,
a socket and anything else the emulator has no name for, so the shipped
message read "there is already a other at that path".
The catch-all stays for :device, where "a device" reads correctly.
Pinned with a fifo at the marker path, which is the one :other a test
can create with no privileges and no file mode - a mode fixture here
would only sometimes describe the state it names.
Co-Authored-By: Claude <noreply@anthropic.com>
Castle.Commands and Castle.Peer both carry failing branches that no case reached. These are the ones a fixture can produce without a file mode, which the suite refuses on the grounds that root and some filesystems ignore one - a fixture that only sometimes describes the state it names is a test that only sometimes tests anything. In Castle.Commands: a version directory that is a file rather than a directory; a node whose which_releases/0 reports neither a current nor a permanent release, which the restart classification has to answer about before the refusal is made; a relup entry the classification cannot read, walked past rather than raised on; a symlink and a fifo at the marker path; a marker already there whose first line is empty; a marker claimed between unclaimed/3 and the publish, where publish/2 refusing rather than replacing is the whole of the defence against the second VM the node-local lock cannot reach; an :enoent from the marker removal rather than from the read; a throw and an exit out of install_release/1 with the marker stranded; and the two indeterminate root comparisons that come from the second path rather than the first. In Castle.Peer: a release file holding more than one term and one that will not parse; a sys.config that will not parse, which is the only reason format_error/1 has a clause for a reason that is not an atom; a dangling symlink at sys.config.pristine, which publish/2 refuses rather than following - File.write/2 there creates the target and fills it with the configuration; a write through a closed handle, which is why written/3 is :file.write/2 and not IO.binwrite/2; a chmod that cannot land because the name has gone; a model whose mode cannot be read; a directory whose mode cannot be read; and a working directory that cannot be created. Every one of these was mutation-checked: the branch it covers was broken, the test was confirmed to fail, and the branch restored. Co-Authored-By: Claude <noreply@anthropic.com>
AGENTS.md gains an account of what mix test --cover measures now that test/support is out of it, the lib-only figure and its per-module breakdown, why the threshold is 85 rather than 90, and why --cover is not part of mix precommit. And the part that closes castle#8: what is left uncovered, branch by branch, with the reason each is a decision. Most are filesystem failures no fixture can arrange without a mode; the rest are the compiler default-argument clauses for arities nothing calls, a target release carrying an older Castle.Peer, and the booted release that is Forecastle e2e work. RELEASE.md records the refusal-message fix, which is the only user-visible outcome here. Co-Authored-By: Claude <noreply@anthropic.com>
Three tests, for two branches the residual-coverage inventory got wrong and one it dismissed. closed/2 was executed but not discriminated. The closed-handle test fails both the write and the close, and `with :ok <- written, :ok <- closed` reports the write - so replacing closed/2 error clause with :ok left the whole file green while the inventory claimed the clause was uncovered and the report showed it hit. Telling the two apart needs a handle whose write succeeds and whose close does not, which is a process: File.io_device/0 is `pid | file_descriptor`, and :file.write/2 sends a pid an io request where File.close/1 sends a file_request. The new test asserts the close own reason and that the file was not narrowed, which is the "mode only once both succeeded" half of fill/3. publish/2 generic error needed none of the modes or mounts it was grouped with: File.ln/2 with a staging file that is not there answers :enoent and creates no destination. call/2 unrecognised answer is the cross-version contract, and reaching it needs a castle application built for the target alone. SyntheticRelease.stub_castle/2 builds one with :compile.forms/2 rather than from Elixir source - the module is named Castle.Peer, so compiling it from source would load the stub over the running one. build/2 gains :override to point lib/<app>-<vsn> at it while leaving the release file alone. Co-Authored-By: Claude <noreply@anthropic.com>
The 85 threshold sat below the 87.95% it was meant to floor, so it ratcheted nothing and licensed a thirteen-line regression. It is now 88.58 - the measured figure, 419 of 473 relevant lines. One uncovered line added to lib gives 88.40% and fails; that was checked rather than assumed. And precommit runs test --cover, so the floor is a gate. Nothing else ran it, and a floor nothing enforces is decoration. CI test matrix stays on a plain mix test: cover line attribution can differ between Elixir versions, so the figure is enforced on the one pinned toolchain the precommit job uses. 90% would need 426 covered, seven more. Five of the seven can only come from the compiler generated clauses for defaulted arities nothing calls - measured, not assumed: calling the intermediate arities covers all five and reaches 89.64%. Each is a delegation whose defaults are a subset of an arity already called, so those cases would asserted nothing. That is the move this project does not make. The mechanism claim was also wrong. cover does not need distribution: :cover.start/0 works in a VM with no node name. What happens is that Mix instruments the modules loaded on this node and the peer loads the target own beams, which nothing instrumented - and cover only route to another VM is a distributed node, which the peer deliberately is not. Co-Authored-By: Claude <noreply@anthropic.com>
The residual-coverage list was written from analysis rather than from the report, and it was wrong three ways: it claimed closed/2 was uncovered while the report showed the clause hit, and it grouped publish/2 generic error and call/2 unrecognised answer with paths needing modes or mounts when neither needs either. All three are now covered, and the list is rebuilt line by line against a regenerated cover/ - stale HTML from before the test/support exclusion is how the inconsistency survived, so the note says to delete it first. The mechanism claim is corrected: cover is not unable to run in a non-distributed VM, it is that Mix instruments this node and nothing instruments the peer. The remaining 21 observable lines are split into the five compiler generated clauses and the sixteen that need a mode, a device node or a provider sabotaging the working directory, with the arithmetic for why 90% is out of reach honestly rather than absolutely. Also records why fill/3 needs three different handles to pin its three steps, and why stub_castle/2 compiles with :compile.forms/2. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 029bf075bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The 88.58 floor was the pinned toolchain exact reading with no slack, and cover line attribution is not constant across the versions elixir: "~> 1.18" invites in. Elixir 1.20 counts the head of defp forward_standard_error do (peer.ex:981) as executable where 1.19 does not, so the denominator goes from 473 to 474 and the same 419 covered lines report 88.40% instead of 88.58%. That failed the mandatory mix precommit on a clean tree for anyone on a current Elixir - and CI never saw it, because the test matrix runs a plain mix test while the single precommit job is pinned to 1.19. Measured, one cell per toolchain with isolated MIX_HOME and MIX_BUILD_ROOT: 1.18.4/27 81.22% 88.58% exit 0 1.19.5/28 81.22% 88.58% exit 0 1.20.3/28 80.84% 88.40% exit 0 1.20.3/29 80.84% 88.40% exit 0 The floor is now 88 - the lowest reading, rounded down so it absorbs a line of drift rather than sitting on one version figure. Negative control on 1.20.3, the least slack of the range: +1 line 88.21% exit 0, +2 88.03% exit 0, +3 87.84% exit 3. On 1.19.5 it absorbs three and fails on the fourth. Both ladders were run, not inferred. That is looser than a floor ideally is, and deliberately so: a floor that fires on a clean tree teaches people to bypass the gate. The previous comment had already said attribution varies by version while the number ignored it, so both comments are corrected too - including the one on the precommit alias, which now says plainly that this runs on whatever Elixir the contributor has. Co-Authored-By: Claude <noreply@anthropic.com>
The floor was enforced in only one place that ran it - mix precommit, on whatever Elixir the contributor happened to have - and nothing in CI checked it at all. The test matrix ran a plain mix test and the precommit job is pinned to the one version that agreed with the number, so CI stayed green while a clean tree failed the gate on Elixir 1.20. A drift nothing watches is a drift the next contributor finds. The newest cell of the test matrix now exports coverage and checks the floor. It is the lowest reporter, so it trips first on attribution drift. Every other cell stays on a plain mix test: seven readings that disagree by version would be noise rather than signal. The include entry augments the existing 1.20/29 cell rather than adding a job. Two steps rather than one mix test --cover, so the step that goes red names what broke without opening the log - exporting cannot fail on the threshold, and mix test.coverage cannot fail on a test. The job name carries the same information. Verified on 1.20.3/OTP 29: export exits 0, the check reports 88.40% and exits 0, and with the floor raised to 89 the check exits 3 - so it is a gate and not decoration. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Refs #8. Targets
release/1.0.0. This is the last gating issue for 1.0.0.Coverage on the shipped code goes 87.95% → 88.58% (419 of 473 relevant lines), 154 → 177 tests, and the figure is now a gate rather than a report. One user-visible bug fixed along the way.
The issue is stale and was deliberately not implemented literally
All three of its premises are gone. The
mix newboilerplate assertingCastle.hello() == :worldwas removed incfd3ee0, before this release work started. Its central suggestion — testgenerate/1, "pure-ish, readsbuild.config, runs config providers" — names a function castle#13 deleted outright along with the wholebuild.configpath. And coverage is not "zero": there were already 154 tests.What survives is the intent: cover the surface honestly, and close #8 with an accurate account of what is and is not covered.
The measurement was wrong before any test was written
mix test --coverwas countingtest/supportas if it were product code. Seven fixture modules are now named individually intest_coverage: [ignore_modules: …]— individually, because a~r/Stub/regex would silently swallow a production module spelled that way.Castle.PeerProviderStubat 4.17% was the visible symptom of a second, deeper artefact that affects production code: 33 ofCastle.Peer's missed lines are everything between## In the peerand## Files—resolve/1, the standard-error relay, the pipeline, the compile-environment check. That code runs in the peer VM, whose beams nothing instrumented, and cover's only route into another VM is a distributed node, which this peer deliberately is not. It executes on every run ofCastle.PeerTestand is counted as missed.Two ways to "fix" that are both wrong and are rejected in writing: calling
resolve/1on the test node runs Elixir's pipeline in the very VM the mechanism exists to keep it out of, and splitting it into its own module to exclude it is impossible because{Castle.Peer, :resolve, 1}is a cross-version contract with the next Castle.An earlier draft of this branch explained that as "cover cannot attach without a node name". That is false —
:cover.start/0succeeds fine withnode() == :nonode@nohost— and it is corrected. The true statement is narrower and is about where instrumentation is applied, not about cover requiring distribution.Bug found and fixed
Castle.Commands.describe_type/1fell through to"a #{type}", andFile.lstat/1answers:otherfor a named pipe or socket — so the shipped refusal read "there is already a other at that path". Fixed with an:otherclause; the catch-all stays for:device, where "a device" reads correctly. Pinned with a fifo at the marker path, the only:othera test can make with no privileges and no mode.da055b7.The threshold: measured, not chosen
The floor is 88.58, the measured figure. Verified as a real gate: adding one uncovered line drops it to 88.40% and
mix precommitexits 3.precommitnow runstest --cover. CI'stestmatrix stays on plainmix test, because cover's line attribution varies by Elixir version and the number is enforced on the one pinned toolchain theprecommitjob uses.An earlier draft set 85 on the argument that the built-in 90 was "unreachable by construction". That was wrong twice over and the adversarial review caught it: 85 sat below the 87.95% it was meant to floor, so it licensed a regression rather than preventing one, and nothing ran
--coverat all, so it gated nothing.90% is reachable, and is deliberately not reached. It needs 7 more covered lines, and 5 of them come only from the compiler's generated clauses for defaulted arities nothing calls. A throwaway probe calling
install/2,install/3,commit/3and friends covered all five and hit 89.64%; it was then deleted. Each is a delegation whose defaults are a strict subset of an arity already exercised, soCastle.install/1incastle_test.exsalready establishes that the defaults are the real modules — a case callinginstall/3asserts nothing new and only moves the number.AGENTS.mdrecords this as arithmetic, not as a claim of impossibility.What the adversarial review changed
Codex returned needs-attention with three medium findings, all real, all fixed. The first is the one worth reading:
A test credited a branch it could not detect breaking. The closed-handle test closed the handle before
fill/3, so both the write and the close failed — and sincewritten/3andclosed/2emit the same"Cannot write <path>."prefix andfill/3doeswith :ok <- written, :ok <- closed, the write error always won. Mutatingclosed/2's error clause to:okleft all assertions passing. Confirmed by running it: 54 tests, 0 failures against broken code. This is exactly the metric-moving pattern the issue needed to avoid, and it had been reported as mutation-checked. The original test is kept (it does pinwritten/3, which was what had actually been checked) and a second one now pinsclosed/2, using a process as the io device:File.io_device/0ispid | file_descriptor, and:file.write/2sends a pid an io request whileFile.close/1sends a file request — two protocols, so the write succeeds and the close refuses. It asserts the close's own reason (:enospc, which the write cannot produce) and that a pre-existing0644file was not narrowed to0600, which is the "chmod only once both succeeded" half of the contract.All 23 new tests were then audited for that shape. One instance, the one above; every other assertion is scoped to a path or reason its sibling branch cannot produce.
AGENTS.mdrecords the trap.The residual-coverage inventory was not reconciled with the report.
publish/2's generic error was grouped with paths needing modes, mounts or cross-device fixtures; it needs none —File.ln(missing_staging, dest)returns{:error, :enoent}on an ordinary tmp dir. Now covered, asserting the error and that nothing was published. Regeneratingcover/from clean surfaced two further errors:closed/2listed as uncovered while the report showed it hit, andcall/2's cross-version refusal dismissed as needing machinery it did not need — also covered now, via a stub castle built with:compile.forms/2rather than Elixir source, because the module is namedCastle.PeerandCode.compile_string/1would load the stub over the running one.The threshold, as above.
Reconciled missed-line inventory (54)
peer.ex959–1079) — unmeasurable, exercised every run.Peerorigin-side — 346stop/1rescue (race); 518/519release_file/1listing (mode); 638keep/2generic publish (write_like/3creates the staging immediately before, so only cross-device remains); 825empty/1listing (unreachable viawork_dir/1); 1147/1161expand/2's two writes (needs a provider sabotaging the working directory).Commands— 592, 593, 1418 default-arg clauses; 1062, 1092, 1103 and 1286, 1288, 1294, 1295 (arm/4's three publish failures and theunarmed/3/detail/1message they share, seven lines standing or falling together); 1243 unreadable marker; 1273describe_type's catch-all (:device, root only).Castle— 316, 317 default-arg clauses.Deliberately out of scope
Booted-release coverage stays in Forecastle's
:e2esuite, which is the de facto integration test for Castle's code: the in-peer section against a real release, the marker consumed by a real launcher,running/1polled across a real reboot,bin/castleexit statuses, andrestart_upgrade_test.exs(OS pid change, rollback, reinstall). None of it is measured here, and duplicatingtest/fixtures/sampleinto Castle is not worth it.A file-mode fixture is refused throughout: root and some filesystems ignore one, so a test resting on it is not deterministic.
Verification
mix precommitgreen — 177 tests, credo--strictclean, coverage gate passed. All 23 new tests mutation-checked across 27 mutations; every mutation broke the branch under test, every test was confirmed to fail, and bothlib/files were verified byte-identical to their pre-mutation state afterwards. Theclosed/2mutation that previously survived was re-run independently and now fails.Note
One test shells out to
mkfifo, unguarded on purpose — a conditional test would violate this repo's "deterministic inputs or nothing" rule. Windows is already unsupported andpeer_test.exsalready shells out tokill. CI runs ubuntu-latest and macos-15, both of which have it.